Skip to content

feat: token transfer operator == adition#1320

Merged
rwalworth merged 6 commits intohiero-ledger:mainfrom
Egbaiyelo:feat/tokentransfer==
Apr 8, 2026
Merged

feat: token transfer operator == adition#1320
rwalworth merged 6 commits intohiero-ledger:mainfrom
Egbaiyelo:feat/tokentransfer==

Conversation

@Egbaiyelo
Copy link
Copy Markdown
Contributor

@Egbaiyelo Egbaiyelo commented Apr 4, 2026

Description:

Implementing the TokenTransfer== operator

Related issue(s):

Fixes #1298

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

Signed-off-by: Egbaiyelo <moteniolaegbaiyelo@trentu.ca>
@Egbaiyelo Egbaiyelo requested review from a team as code owners April 4, 2026 17:01
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 4, 2026

Hey @Egbaiyelo 👋 thanks for the PR!
I'm your friendly PR Helper Bot 🤖 and I'll be riding shotgun on this one, keeping track of your PR's status to help you get it approved and merged.

This comment updates automatically as you push changes -- think of it as your PR's live scoreboard!
Here's the latest:


PR Checks

DCO Sign-off -- All commits have valid sign-offs. Nice work!


GPG Signature -- All commits have verified GPG signatures. Locked and loaded!


Merge Conflicts -- No merge conflicts detected. Smooth sailing!


Issue Link -- Linked to #1298 (assigned to you).


🎉 All checks passed! Your PR is ready for review. Great job!

@github-actions github-actions bot added the status: needs revision A pull request that requires changes before merge label Apr 4, 2026
@codacy-production
Copy link
Copy Markdown

codacy-production bot commented Apr 4, 2026

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 5 complexity

Metric Results
Complexity 5

View in Codacy

TIP This summary will be updated as you push new changes. Give us feedback

Signed-off-by: Egbaiyelo <moteniolaegbaiyelo@trentu.ca>
Copy link
Copy Markdown
Contributor

@rwalworth rwalworth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @Egbaiyelo! There are a few things to address before this can be merged:

Issue link fix: The PR description has Fixes # 1298 (space after #) - GitHub won't auto-link that, which is why the bot flagged it. Please update it to Fixes #1298 (no space).

I left a few comments below on the code changes. Let me know if you have any questions!

@github-actions github-actions bot added status: needs review The pull request is ready for maintainer review and removed status: needs revision A pull request that requires changes before merge labels Apr 5, 2026
@Egbaiyelo
Copy link
Copy Markdown
Contributor Author

I genuinely cant run the tests for some reason? something about hproto-populate?

Signed-off-by: Egbaiyelo <moteniolaegbaiyelo@trentu.ca>
@Egbaiyelo Egbaiyelo force-pushed the feat/tokentransfer== branch from 1d2345c to b79455b Compare April 5, 2026 01:19
Signed-off-by: Egbaiyelo <moteniolaegbaiyelo@trentu.ca>
@Egbaiyelo
Copy link
Copy Markdown
Contributor Author

@rwalworth So I am a bit confused with the test cases, i came back realising I might not have done the right thing, I think they cover everything now but I dont see the structure of what I did in the codebase

Copy link
Copy Markdown
Contributor

@rwalworth rwalworth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @Egbaiyelo — good progress on the SPDX header fix and adding the //----- separator! There are still a couple of things to address before this can merge, the most critical being that the operator== implementation in TokenTransfer.cc is a free function rather than a member function, which means it won't compile. I've left the details in the comments below.

@rwalworth
Copy link
Copy Markdown
Contributor

@rwalworth So I am a bit confused with the test cases, i came back realising I might not have done the right thing, I think they cover everything now but I dont see the structure of what I did in the codebase

The tests look good - they cover all three cases required by the issue:

  1. Two default-constructed instances are equal ✓
  2. Two identically-constructed instances are equal ✓
  3. Instances differing in each field are not equal ✓

The structure (going straight from // Given to // Then without an explicit // When) is a bit different from some of the other tests in the file, but it's perfectly fine for equality assertions - the test name makes the intent clear.

One minor thing: in OperatorEqualsDiff, testTokenTransfer is constructed with the hardcoded value true for mIsApproval. Since getTestIsApproval() also returns true, they're equivalent - using getTestIsApproval() would be slightly more consistent with the rest of the test, but it's not a blocker.

The reason you can't run the tests right now is the compilation error in TokenTransfer.cc - the operator== is a free function that can't access the member variables. Once that's fixed (see the thread on that file), things should build and the tests should pass!

@rwalworth
Copy link
Copy Markdown
Contributor

I genuinely cant run the tests for some reason? something about hproto-populate?

Can you share the full error message? An error mentioning "hproto-populate" sounds like a build system or dependency issue unrelated to your changes - possibly a missing proto generation step or a stale build cache.

@rwalworth rwalworth added status: needs revision A pull request that requires changes before merge and removed status: needs review The pull request is ready for maintainer review labels Apr 5, 2026
@Egbaiyelo
Copy link
Copy Markdown
Contributor Author

ill rebuild it actually

Signed-off-by: Egbaiyelo <moteniolaegbaiyelo@trentu.ca>
@github-actions github-actions bot added status: needs review The pull request is ready for maintainer review and removed status: needs revision A pull request that requires changes before merge labels Apr 5, 2026
Copy link
Copy Markdown
Contributor

@rwalworth rwalworth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sticking with this @Egbaiyelo - the implementation and test coverage are looking good now! The operator== logic is correct, the //----- separator is there, the member function form is right, and the SPDX header is clean. Just one small doc comment fix needed in the header before this can merge.

@rwalworth rwalworth removed the status: needs review The pull request is ready for maintainer review label Apr 5, 2026
@rwalworth rwalworth added the status: needs revision A pull request that requires changes before merge label Apr 5, 2026
Signed-off-by: Egbaiyelo <moteniolaegbaiyelo@trentu.ca>
@github-actions github-actions bot added status: needs review The pull request is ready for maintainer review and removed status: needs revision A pull request that requires changes before merge labels Apr 8, 2026
Copy link
Copy Markdown
Contributor

@rwalworth rwalworth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - all the blocking issues have been resolved! Running the workflows now and will merge once they pass!

@rwalworth rwalworth merged commit 993d0e8 into hiero-ledger:main Apr 8, 2026
11 checks passed
@rwalworth rwalworth removed the status: needs review The pull request is ready for maintainer review label Apr 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Beginner]: Add operator== to TokenTransfer

2 participants